test/e2e/testnet/node.go (1)

308-308: Remove unused parameter ctx in RemoteAddressGRPC

The ctx parameter in RemoteAddressGRPC is not used. Consider removing it to simplify the method signature.

Apply this diff to remove the unused parameter:

- func (n Node) RemoteAddressGRPC(ctx context.Context) (string, error) {
+ func (n Node) RemoteAddressGRPC() (string, error) {

- hostName := n.Instance.Network().HostName()
- return fmt.Sprintf("%s:%d", hostName, grpcPort), nil
+ hostName := n.Instance.Network().HostName()
+ return fmt.Sprintf("%s:%d", hostName, grpcPort), nil
}

[warning]

Note: Static analysis tool flagged this issue at line 308.

🧰 Tools

🪛 golangci-lint (1.62.2)

[warning] 308-308: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _

(revive)

🪛 GitHub Check: lint / golangci-lint

[failure] 308-308:
unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Check the ownership of the keyring directory in the txsim instance.

# Expected: The owner and group should be 10001.

kubectl exec <txsim_instance_name> -- ls -ld /home/celestia/.celestia-app/keyring-test

#!/bin/bash
# Let's check the Dockerfile or container configuration to verify the user ID
fd Dockerfile | xargs cat

# Also check for any user configuration in deployment files
rg -A 5 "user:" 

#!/bin/bash
# Description: Ensure that txsimKeyringDir contents are present in the txsim instance storage.

# Expected: The keyring files should exist in the specified remote directory.
# Replace <txsim_instance_name> with the actual instance name.

kubectl exec <txsim_instance_name> -- ls /home/celestia/.celestia-app/keyring-test

#!/bin/bash
# Let's analyze the code around the temporary directory usage to understand how txsimKeyringDir is used

# First, let's find the full context of the function
rg -B 10 -A 20 'tmpDir, err := os\.MkdirTemp\("", "e2e_test_"\)' test/e2e/testnet/testnet.go

# Also check for any usage of txsimKeyringDir
rg "txsimKeyringDir" test/e2e/testnet/testnet.go

-func MakeConfig(ctx context.Context, node *Node, opts ...Option) (*config.Config, error) {
+func MakeConfig(ctx context.Context, node *Node, peers []string, opts ...Option) (*config.Config, error) {

- cfg, err := MakeConfig(ctx, n, configOptions...)
+ cfg, err := MakeConfig(ctx, n, peers, configOptions...)

#!/bin/bash
# Description: Check release notes for updated golang.org/x packages.

packages=(
  "[email protected]"
  "[email protected]"
  "[email protected]"
  "[email protected]"
  "[email protected]"
  "[email protected]"
)

for pkg in "${packages[@]}"
do
  echo "Checking $pkg for breaking changes..."
  # Open the release notes page for manual review.
  xdg-open "https://pkg.go.dev/golang.org/x/${pkg%%@*}/v0.0.0-${pkg#*@}"
done

#!/bin/bash
# Let's check how these dependencies are used in the codebase
for pkg in crypto net sync sys term text; do
  echo "=== Checking usage of golang.org/x/$pkg ==="
  rg "\"golang\.org/x/$pkg\"" -A 2
done

# Also check if there are any Go files that might be affected
echo -e "\n=== Checking Go files that import these packages ==="
fd -e go --exec grep -l "golang.org/x/"

#!/bin/bash
# Let's check the specific imports in the identified files
echo "=== Checking imports in payforblob.go ==="
head -n 20 ./x/blob/types/payforblob.go | grep "golang.org/x/"

echo -e "\n=== Checking imports in data_availability_header.go ==="
head -n 20 ./pkg/da/data_availability_header.go | grep "golang.org/x/"

# Let's also check for indirect dependencies in go.mod
echo -e "\n=== Checking indirect dependencies in go.mod ==="
grep "golang.org/x/" go.mod

test/e2e/testnet/util.go (1)

Line range hint 14-30: LGTM! Consider enhancing error handling.

The migration from zerolog to the standard logger is well-implemented. However, consider adding debug-level logging for the actual values when environment variables are found (excluding the token for security).

 func GetGrafanaInfoFromEnvVar(logger *log.Logger) *GrafanaInfo {
 	logger.Println("Checking Grafana environment variables")
 	if os.Getenv("GRAFANA_ENDPOINT") == "" ||
 		os.Getenv("GRAFANA_USERNAME") == "" ||
 		os.Getenv("GRAFANA_TOKEN") == "" {
 		logger.Println("No Grafana environment variables found")
 		return nil
 	}
 	logger.Println("Grafana environment variables found")
+	logger.Printf("Grafana endpoint: %s, username: %s\n", 
+		os.Getenv("GRAFANA_ENDPOINT"), 
+		os.Getenv("GRAFANA_USERNAME"))
 	return &GrafanaInfo{

test/e2e/testnet/txsimNode.go (2)

31-31: LGTM! Consider enhancing error logging.

The migration to standard logger is well-implemented. However, the error logging could be more detailed to aid in debugging.

-		logger.Println("failed to set image for tx client", "name", name, "image", image, "error", err)
+		logger.Printf("failed to set image for tx client %s: %v\n", name, err)

Also applies to: 50-54

---

Line range hint 28-93: Consider breaking down the CreateTxClient function.

The function is handling multiple responsibilities including instance creation, resource configuration, and argument building. Consider breaking it down into smaller, focused functions for better maintainability.

Example structure:

func createInstance(ctx context.Context, knuu *knuu.Knuu, name, image string) (*instance.Instance, error) {
    // Instance creation logic
}

func configureResources(instance *instance.Instance, resources Resources, volumePath string) error {
    // Resource configuration logic
}

func buildTxSimArgs(endpoint string, seed int64, config TxSimConfig) []string {
    // Argument building logic
}

test/e2e/simple.go (1)

Line range hint 1-93: Consider parameterizing test duration and transaction threshold

The hardcoded values for test duration (30 seconds) and minimum transactions (10) could be made configurable to allow for different test scenarios.

Consider adding these as parameters or constants:

 func E2ESimple(logger *log.Logger) error {
     const (
         testName = "E2ESimple"
+        testDuration = 30 * time.Second
+        minExpectedTxs = 10
     )
     // ... rest of the code ...
-    logger.Println("Waiting for 30 seconds to produce blocks")
-    time.Sleep(30 * time.Second)
+    logger.Printf("Waiting for %v to produce blocks", testDuration)
+    time.Sleep(testDuration)
     // ... rest of the code ...
-    if totalTxs < 10 {
-        return fmt.Errorf("expected at least 10 transactions, got %d", totalTxs)
+    if totalTxs < minExpectedTxs {
+        return fmt.Errorf("expected at least %d transactions, got %d", minExpectedTxs, totalTxs)
     }

test/e2e/minor_version_compatibility.go (1)

Line range hint 143-152: Enhance version retrieval robustness

The getAllVersions function could benefit from additional error handling and validation.

Consider this enhanced implementation:

 func getAllVersions() (string, error) {
+    // Ensure we're in a git repository
+    if _, err := exec.Command("git", "rev-parse", "--git-dir").Output(); err != nil {
+        return "", fmt.Errorf("not in a git repository: %v", err)
+    }
+
     cmd := exec.Command("git", "tag", "-l")
     output, err := cmd.Output()
     if err != nil {
         return "", fmt.Errorf("failed to get git tags: %v", err)
     }
-    allVersions := strings.Split(strings.TrimSpace(string(output)), "\n")
+    versions := strings.Split(strings.TrimSpace(string(output)), "\n")
+    if len(versions) == 0 {
+        return "", fmt.Errorf("no git tags found")
+    }
-    return strings.Join(allVersions, " "), nil
+    return strings.Join(versions, " "), nil
 }

test/e2e/major_upgrade_v2.go (1)

Line range hint 122-141: Optimize getHeight function with context timeout

The getHeight function could be simplified by using context timeout directly instead of manual timer management.

Consider this more idiomatic implementation:

 func getHeight(ctx context.Context, client *http.HTTP, period time.Duration) (int64, error) {
-    timer := time.NewTimer(period)
+    ctx, cancel := context.WithTimeout(ctx, period)
+    defer cancel()
+
     ticker := time.NewTicker(100 * time.Millisecond)
+    defer ticker.Stop()
+
     for {
         select {
-        case <-timer.C:
-            return 0, fmt.Errorf("failed to get height after %.2f seconds", period.Seconds())
+        case <-ctx.Done():
+            return 0, fmt.Errorf("failed to get height after %.2f seconds: %v", period.Seconds(), ctx.Err())
         case <-ticker.C:
             status, err := client.Status(ctx)
             if err == nil {
                 return status.SyncInfo.LatestBlockHeight, nil
             }
             if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
                 return 0, err
             }
         }
     }
 }

test/e2e/benchmark/benchmark.go (1)

45-46: Consider enhancing error logging.

While the logger parameter is correctly passed to internal functions, consider using it for error logging instead of returning raw errors.

 testNet, err := testnet.New(logger, kn, testnet.Options{
   Grafana:          testnet.GetGrafanaInfoFromEnvVar(logger),
   ChainID:          manifest.ChainID,
   GenesisModifiers: manifest.GetGenesisModifiers(),
 })
-testnet.NoError("failed to create testnet", err)
+if err != nil {
+  logger.Printf("failed to create testnet: %v", err)
+  return nil, fmt.Errorf("failed to create testnet: %w", err)
+}

test/e2e/testnet/node.go (1)

281-283: Consider keeping context parameter for future extensibility

While using hostname instead of IP is a good change for container environments, removing the context parameter might limit future extensibility (e.g., for timeouts, cancellation, or tracing).

Also applies to: 304-306, 310-312, 319-321

test/e2e/testnet/testnet.go (2)

466-488: Add documentation for lazy loading behavior

The GenesisHash implementation uses lazy loading, which should be documented for better maintainability.

Add a comment explaining the lazy loading behavior:

+// GenesisHash returns the hash of the genesis block.
+// The hash is lazily loaded on the first call and cached for subsequent calls.
 func (t *Testnet) GenesisHash(ctx context.Context) (string, error) {

---

378-392: Consider making retry parameters configurable

The retry logic uses hardcoded values (20 attempts, linear backoff). Consider making these parameters configurable for different environments.

+const (
+    defaultSyncRetries = 20
+    defaultSyncBackoff = time.Second
+)
+
+type SyncConfig struct {
+    MaxRetries int
+    BackoffDuration time.Duration
+}

🧰 Tools

🪛 GitHub Check: test / test-short

[failure] 387-387:
(*log.Logger).Printf call has arguments but no formatting directives

🪛 GitHub Check: test / test-race

[failure] 387-387:
(*log.Logger).Printf call has arguments but no formatting directives

🪛 GitHub Check: lint / golangci-lint

[failure] 387-387:
printf: (*log.Logger).Printf call has arguments but no formatting directives (govet)

Configuration used: .coderabbit.yaml
Review profile: CHILL

test/e2e/major_upgrade_v2.go (1)

Line range hint 84-93: Consider enhancing version transition logging

The version verification logic is solid, but consider adding more detailed logging around the version transitions to make debugging easier in case of failures.

        resp, err := client.Header(ctx, &heightBefore)
        testnet.NoError("failed to get header", err)
-       logger.Println("Node", i, "is running on version", resp.Header.Version.App)
+       logger.Printf("Node %d version before upgrade (height %d): expected=%d, actual=%d", 
+           i, heightBefore, v1.Version, resp.Header.Version.App)
        if resp.Header.Version.App != v1.Version {
            return fmt.Errorf("version mismatch before upgrade: expected %d, got %d", v1.Version, resp.Header.Version.App)
        }

        resp, err = client.Header(ctx, &upgradeHeight)
        testnet.NoError("failed to get header", err)
+       logger.Printf("Node %d version after upgrade (height %d): expected=%d, actual=%d",
+           i, upgradeHeight, v2.Version, resp.Header.Version.App)
        if resp.Header.Version.App != v2.Version {
            return fmt.Errorf("version mismatch after upgrade: expected %d, got %d", v2.Version, resp.Header.Version.App)
        }

test/e2e/testnet/testnet.go (1)

148-152: Use a more specific pattern for temporary directory.

The current pattern "e2e_test_" could be made more specific to help with debugging and cleanup.

Apply this diff to use a more specific pattern:

-	tmpDir, err := os.MkdirTemp("", "e2e_test_")
+	tmpDir, err := os.MkdirTemp("", fmt.Sprintf("celestia_e2e_test_%s_", name))

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Search for all calls to RemoteGRPCEndpoints to ensure consistent usage
rg -A 2 "RemoteGRPCEndpoints\(" test/

# Search for the method definition to verify the intended signature
ast-grep --pattern 'func $_($$) RemoteGRPCEndpoints($$$) {
  $$$
}'

test/e2e/testnet/testnet.go (2)

29-39: Consider removing redundant chainID field

The chainID field appears to be redundant since it's already stored in the genesis object. This could lead to inconsistencies if the values get out of sync.

Consider removing the chainID field and using t.genesis.ChainID() instead.

---

148-153: Consider moving defer after error checks

The defer os.RemoveAll(tmpDir) might execute too early if subsequent operations fail. This could make debugging harder as the temporary files would be removed before they can be inspected.

Consider moving the defer statement after all error checks or passing the temporary directory path to a cleanup function that can be called on success.

Configuration used: .coderabbit.yaml
Review profile: CHILL

 func (t *Testnet) GenesisHash(ctx context.Context) (string, error) {
+    if ctx.Err() != nil {
+        return "", ctx.Err()
+    }
     if t.genesisHash == "" {
         if t.knuu == nil {
             return "", errors.New("knuu is not initialized")

 func New(logger *log.Logger, knuu *knuu.Knuu, opts Options) (*Testnet, error) {
+    if logger == nil {
+        return nil, errors.New("logger cannot be nil")
+    }
     opts.setDefaults()
     return &Testnet{

test/e2e/testnet/testnet.go (3)

148-153: Enhance error messages with more context

While the temporary directory handling is robust, the error messages could be more descriptive to aid in debugging.

Apply this diff to improve error messages:

 tmpDir, err := os.MkdirTemp("", "e2e_test_")
 if err != nil {
-    return fmt.Errorf("failed to create temp dir: %w", err)
+    return fmt.Errorf("failed to create temp dir for txsim %s: %w", name, err)
 }

---

380-396: Consider capping the exponential backoff

The exponential backoff (1<<uint(i)) grows very quickly. By the 10th iteration, the sleep duration would be 1024 seconds.

Apply this diff to cap the backoff:

-time.Sleep(time.Second * time.Duration(1<<uint(i)))
+backoff := time.Second * time.Duration(1<<uint(i))
+maxBackoff := time.Second * 60
+if backoff > maxBackoff {
+    backoff = maxBackoff
+}
+time.Sleep(backoff)

---

117-120: Enhance log messages with actionable information

While the logging is consistent, some messages could be more actionable by including additional context or suggesting next steps.

Example improvements:

-t.logger.Println("txsim creation failed", "name", name, "grpc_endpoint", grpcEndpoint, "error", err)
+t.logger.Println("txsim creation failed, check endpoint availability", "name", name, "grpc_endpoint", grpcEndpoint, "error", err)

-t.logger.Println("failed to start and create proxy", "name", node.Name, "version", node.Version, "error", err)
+t.logger.Println("failed to start and create proxy, verify network connectivity", "name", node.Name, "version", node.Version, "error", err)

Also applies to: 193-194, 218-221, 424-427

Configuration used: .coderabbit.yaml
Review profile: CHILL

 func (t *Testnet) GenesisHash(ctx context.Context) (string, error) {
+    if ctx.Err() != nil {
+        return "", ctx.Err()
+    }
     if t.genesisHash == "" {
         if t.knuu == nil {
             return "", errors.New("knuu is not initialized")

 func New(logger *log.Logger, knuu *knuu.Knuu, opts Options) (*Testnet, error) {
+    if logger == nil {
+        return nil, errors.New("logger cannot be nil")
+    }
     opts.setDefaults()
     return &Testnet{